feat: keep node recovery task running across epoch changes#3516
feat: keep node recovery task running across epoch changes#3516halfprice wants to merge 3 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f83f6a6b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7f83f6a to
dbc0bc0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbc0bc016e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When an epoch change starts while node recovery is in progress, the recovery task was canceled and restarted from scratch, re-scanning the full blob info table on every epoch change even though the previous progress was still valid. The recovery task now keeps running across epoch changes: - The epoch-change path only advances the recovery target (RecoveryInProgress epoch) and starts shard syncs for gained shards; both happen under a new status mutex so that they are atomic with respect to the recovery task's completion. A new task is only started if the running task completed concurrently with the epoch change or stopped unexpectedly; a spurious start is harmless since the extra task exits once it observes that the node is already active. - The task's scan bound stays frozen at the epoch it was started with: blobs certified later are covered by event processing, and shards gained at later epoch changes are covered by shard sync, which the task waits for. - On completion, the task attests epoch sync done for the recovery target currently recorded in the node status (which epoch changes advance), instead of the frozen spawn epoch whose attestation would be dropped as stale. - Attestation requires a clean scan pass during which no shard sync started, tracked by a new shard sync generation counter: a shard sync that starts and terminally fails during a pass leaves missing blobs behind that only a new scan pass finds. The blob scan also pauses promptly when a shard sync starts mid-pass, since per-blob recovery would redundantly decode slivers for shards that shard sync copies in bulk. The shard-gain simtest now also asserts that epoch changes processed while recovering do not restart the recovery task.
…target Addresses review feedback on the persistent recovery task: - Serialize the catch-up restart path with recovery completion: a recovery task from before the node started catching up only scanned blobs certified before its own start epoch, and blob certified events were skipped while catching up, so it must not complete the recovery target written by the catch-up path. The catch-up path now holds the recovery status mutex across writing its recovery target and aborting the previous task; a stale task either observes the RecoveryCatchUp status and exits without attesting, or is aborted before it can complete. - Use a dedicated scan_pass_interrupted flag instead of overloading has_more_blobs when a shard sync interrupts a scan pass, so that removing the verification re-scan (WAL-669) leaves the interrupted- pass signal intact.
dbc0bc0 to
a17f8e4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ch sync done Hold the recovery status mutex through the shards_to_lock loop in process_shard_changes_in_new_epoch_while_recovering. Previously the guard was dropped after starting shard syncs but before locking lost shards, so on an epoch change with lost shards only, the running recovery task could complete and attest epoch sync done while the node still accepted slivers for shards it no longer owns. This matches the catch-up path, which already locks shards under the guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
walrus/crates/walrus-service/src/node/node_recovery.rs
Lines 135 to 137 in 0af33cc
If a node gains a shard in a later epoch and that shard's sync terminates unsuccessfully, the generation check only triggers another pass using the frozen certified_before_epoch bound. Blobs certified after this task began are excluded, yet their original BlobCertified handling had checked the then-current shard assignment and skipped syncing them (blob_event_processor.rs:194-240), before the node owned the new shard. Consequently, recovery can transition to Active and attest the advanced epoch while that gained shard still lacks those blobs; restart recovery with a newer bound (or extend the scan bound) after such a sync failure.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Description
Follow-up to #3464. When an epoch change started while node recovery was in progress, the recovery task was canceled and restarted from scratch — re-scanning the full blob info table on every epoch change even though the previous progress was still valid.
The recovery task now keeps running across epoch changes:
RecoveryInProgressepoch) and starts shard syncs for gained shards, both under a new status mutex so they are atomic with respect to the recovery task's completion. A new task is only started if the running one completed concurrently or stopped unexpectedly.epoch_sync_donefor the current recovery target recorded in the node status (advanced by epoch changes), instead of the frozen spawn epoch, whose attestation would be dropped as stale.Test plan
test_node_recovery_across_epoch_change_with_shard_gainnow additionally asserts the recovery task is spawned exactly once across the epoch changes processed while recovering. Passes on seeds 1 and 2.test_long_node_recovery,test_recovery_in_progress_with_node_restart,test_lagging_node_recovery.walrus-serviceunit tests pass; clippy and fmt clean.